logout user in the case that the underlying connection gets disconnected#383
logout user in the case that the underlying connection gets disconnected#383JacobBarthelmeh wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR ensures that an authenticated user session is properly logged out when the underlying communication channel is abruptly disconnected (not just on a clean COMM_CLOSE). It also defensively clears any stale auth session state during wh_Server_Init, in case the externally-owned auth context carries a leftover session from a prior connection.
Changes:
- In
wh_Server_SetConnected, invokewh_Auth_Logouton the transition toWH_COMM_DISCONNECTEDwhen a user is currently active. - In
wh_Server_Init, after binding the externally-owned auth context, log out any stale active user; fall back to zeroing the user struct if logout fails. - Add
_whTest_Auth_AbruptDisconnecttest that uses a wrappingtest_Logoutcallback to verify the disconnect path triggers exactly one logout and that repeated disconnects are no-ops.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/wh_server.c | Adds logout-on-disconnect in wh_Server_SetConnected and stale-session cleanup in wh_Server_Init. |
| test/wh_test_auth.c | Adds a logout-counting callback and a memory-transport test verifying logout fires on abrupt disconnect and is idempotent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JacobBarthelmeh
force-pushed
the
auth_connection
branch
from
d2a889c to
5985499
Compare
JacobBarthelmeh
force-pushed
the
auth_connection
branch
from
5985499 to
1109abb
Compare
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
LGTM. A few questions and a nit but nothing that would block merge
| * (mirrors the defensive clear in wh_Server_Init). */ | ||
| if (logout_rc != WH_ERROR_OK || | ||
| server->auth->user.user_id != WH_USER_ID_INVALID) { | ||
| memset(&server->auth->user, 0, sizeof(server->auth->user)); |
There was a problem hiding this comment.
Consider unconditionally zeroing this. Also above too. Losing privileges should be no-fail, just like destructors. Gaining privileges is a must-pass.
There was a problem hiding this comment.
Added wh_Auth_Reset for an internal to wolfHSM user reset function and called it unconditionally. It also tries to make use of the lock when available before calling memset.
billphipps
left a comment
billphipps
left a comment
There was a problem hiding this comment.
Please consider unconditionally zeroing userid when logging out. Please resolve additional comments.
JacobBarthelmeh
dismissed stale reviews from billphipps and bigbrett
via
15cb710
|
@bigbrett and @billphipps , I believe I've addressed the current feedback. Thank you for reviewing |
| } | ||
|
|
||
| /* Best-effort lock, but clear the session regardless */ | ||
| rc = WH_AUTH_LOCK(context); |
There was a problem hiding this comment.
@JacobBarthelmeh Seems odd to still clear the session if you can't acquire the lock - this means someone else could be in process of reading. Perhaps the backend would get put into a weird state if synchronous access isn't respected? I'd rather just hard fail if you can't acquire the lock, and let the caller deal with it since it probably means their entire system is broken? I guess you could make the argument that the locking function really shouldn't ever fail, but kind of a bucket of worms. Thoughts?
Follow up to item 4 from #270